Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

fix/http: add logging middleware to default exported external http clients #64402

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ggilmore
Copy link
Contributor

@ggilmore ggilmore commented Aug 10, 2024

Closes https://linear.app/sourcegraph/issue/SRC-507/make-sure-that-the-http-retries-log-some-info-about-requests-that-are

The core change in this PR is the following:

diff --git a/internal/httpcli/client.go b/internal/httpcli/client.go
index 0f1fef2df81..e464afd5a53 100644
--- a/internal/httpcli/client.go
+++ b/internal/httpcli/client.go
@@ -100,7 +100,9 @@ var CachedTransportOpt = NewCachedTransportOpt(redisCache, true)
 // WARN: Clients from this factory cache entire responses for etag matching. Do not
 // use them for one-off requests if possible, and definitely not for larger payloads,
 // like downloading arbitrarily sized files! See UncachedExternalClientFactory instead.
-var ExternalClientFactory = NewExternalClientFactory()
+func ExternalClientFactory(logger log.Logger) *Factory {
+	return NewExternalClientFactory(NewLoggingMiddleware(logger))
+}
 
 // UncachedExternalClientFactory is a httpcli.Factory with common options
 // and middleware pre-set for communicating with external services, but with caching
@@ -175,7 +177,10 @@ func newExternalClientFactory(cache bool, testOpt bool, middleware ...Middleware
 // WARN: This client caches entire responses for etag matching. Do not use it for
 // one-off requests if possible, and definitely not for larger payloads, like
 // downloading arbitrarily sized files! See UncachedExternalDoer instead.
-var ExternalDoer, _ = ExternalClientFactory.Doer()
+func ExternalDoer(logger log.Logger) Doer {
+	d, _ := ExternalClientFactory(logger).Doer()
+	return d
+}
 
 // UncachedExternalDoer is a shared client for external communication. This is a
 // convenience for existing uses of http.DefaultClient.
@@ -204,7 +209,10 @@ var TestExternalDoer, _ = TestExternalClientFactory.Doer()
 // WARN: This client caches entire responses for etag matching. Do not use it for
 // one-off requests if possible, and definitely not for larger payloads, like
 // downloading arbitrarily sized files! See UncachedExternalClient instead.
-var ExternalClient, _ = ExternalClientFactory.Client()
+func ExternalClient(logger log.Logger) *http.Client {
+	c, _ := ExternalClientFactory(logger).Client()
+	return c
+}
 
 // UncachedExternalClient returns a shared client for external communication. This is
 // a convenience for existing uses of http.DefaultClient.

We already had existing HTTP middleware that logs retries, but many of the default exports (default Doer, client, etc.) didn't include this middleware by default:

https://github.com/sourcegraph/sourcegraph/blob/6136978b18306b5e41c77b5704a71da135db5ea9/internal/httpcli/client.go#L363-L412

This PR does all the grunt work of changing these default exports to be a function that accepts a logger (so that we can include the logging middleware by default). The vast majority of the changes are just passing new loggers as appropriate and fixing the resulting type errors.

Test plan

  • Existing unit tests
  • Ran env SRC_LOG_LEVEL=debug sg start and saw debug log lines from the middleware. Ex:
[         worker] DEBUG authz.BitbucketServerAuthzProvider.BitbucketServerClient.BitbucketServerClient.httpcli httpcli/client.go:407 request {"TraceId": "81ae8c7072115329d94228b0887e8776", "SpanId": "444efd3fe6a2e0a8", "host": "bitbucket.sgdev.org", "path": "/rest/api/1.0/repos", "duration": "1.679952667s", "code": 200}

Changelog

@cla-bot cla-bot bot added the cla-signed label Aug 10, 2024
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ggilmore and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels Aug 10, 2024
@ggilmore ggilmore force-pushed the graphite-ggilmorefix_http_add_logging_middleware_to_default_exported_external_http_clients branch 12 times, most recently from 9e2f254 to 71fdef7 Compare August 13, 2024 00:54
@ggilmore ggilmore force-pushed the graphite-ggilmorefix_http_add_logging_middleware_to_default_exported_external_http_clients branch from 71fdef7 to bfbd09e Compare August 13, 2024 05:43
@ggilmore ggilmore marked this pull request as ready for review August 13, 2024 05:48
@ggilmore ggilmore requested a review from a team August 13, 2024 05:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant